-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: adding white label event capture for sharing #27140
Conversation
6510357
to
53e5a50
Compare
Size Change: +333 B (0%) Total Size: 9.44 MB ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sharing logic is used across multiple resource types (dashboard, insight, recordings) and it looks like we're already triggering events for dashboard sharing so it might be best to follow that pattern and expand for insights and recordings.
See setIsEnabled
listener for the dashboard trigger and SharingLogicProps
for the different id props passed in.
aa57343
to
7ec7db3
Compare
Based on the feedback abo e,
I've customized the kea action setConfigValue to log when the user toggles the whitelabel button. I didn't think loaders were necessary here, as this action doesn't trigger any API calls and is handled entirely on the frontend. I'll add the changes for insights and surveys next. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two suggestions but if those are addressed this looks good.
@@ -174,9 +174,9 @@ export function SharingModalContent({ | |||
</div> | |||
} | |||
onChange={() => | |||
guardAvailableFeature(AvailableFeature.WHITE_LABELLING, () => | |||
onChange(!value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should still call onChange
here so if we ever save this to the API the form state is updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we decide to save this to the API, i think we should be able to handle it in the sharingLogic which keeps track of the whitelabel state. I prefer not having onChange and setEmbedConfigValue together since they will be redundant. Let me know if I'm not understanding your suggestion correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setEmbedConfigValue
is only calling eventUsageLogic.actions.reportDashboardWhitelabelToggled(value)
so it is not updating the form state in kea.
d3c9d39
to
bea214b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pending comment
@@ -174,9 +174,9 @@ export function SharingModalContent({ | |||
</div> | |||
} | |||
onChange={() => | |||
guardAvailableFeature(AvailableFeature.WHITE_LABELLING, () => | |||
onChange(!value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we decide to save this to the API, i think we should be able to handle it in the sharingLogic which keeps track of the whitelabel state. I prefer not having onChange and setEmbedConfigValue together since they will be redundant. Let me know if I'm not understanding your suggestion correctly?
Problem
In this PR, I'm adding whitelabel capture event for sharing dashboards
In the following PR's,
I will add changes to
Changes
The event capture happens in the frontend since this feature is purely built in the frontend.
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Does this work well for both Cloud and self-hosted?
Cloud: Yes
How did you test this code?
Shared a dashboards, and toggled the whitelabel button. Verified logs showed up: